-
Notifications
You must be signed in to change notification settings - Fork 547
Implement Scope->getPhpVersion() #3642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0631d67 to
1a464ba
Compare
|
sending it up for review to sync our expectations. if this works for you, I can add it to all rules. when rules take the php-version from the |
src/Analyser/MutatingScope.php
Outdated
| if (!is_int($scalar)) { | ||
| throw new ShouldNotHappenException(); | ||
| } | ||
| $ints[] = $scalar; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this piece of code I realized it's time to do an idea I had for a long time. Please follow my train of thought (while I'm typing this out on a train to Prague :))
Often there's not a single PHP version, but a range of them so returning bool from a method like this isn't exactly right:
public function supportsNullCoalesceAssign(): bool
{
return $this->versionId >= 70400;
}If we're analysing against both PHP 7.3 and 7.4+, we MIGHT support null coalesce assign.
If we're analysing only against PHP 7.3, we do NOT support null coalesce assign.
If we're analysing only against PHP 7.4+, we DO support null coalesce assign.
Sounds familiar? :) We should return TrinaryLogic instead of bool.
But I don't want the headache of BC breaks. Adding a new method about PHP version to Scope allows us to return something else than PhpVersion. A new unrelated class called something like TrinaryPhpVersion or PhpVersions. And we do not need to add all methods from PhpVersion on it. We can add them slowly as needed.
So in this PR this new class could have just producesWarningForFinalPrivateMethods(): TrinaryLogic correctly computed for all various scenarious of what PHP_VERSION_ID can be in that place.
And the rule would correctly handle yes/no/maybe along with some tests.
The constructor of this class could just accept a list of supported PHP versions like 80000, 80100, 80200.... Because all methods in PhpVersion asking about this number always ask about the minor version, ending in 00. Meaning that if PHP_VERSION_ID is 80210, the 10 at the end would have to be trimmed and exchanged for 00 to correctly answer.
Does this sound compelling to you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I can follow this train :).
when rules take the php-version from the
Scope, we also need to think how we can/want control thePhpVersionin tests, so we can testPhpVersiondependent behaviour
this is still a open question though. how to adjust existing tests so we can force the php version?
wdyt about adding RuleTestCase->getPhpVersions() with a default impl and handle it similar to getCollectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need anything like that. If we want to test behaviour for a specific PHP version we will just write the condition if (PHP_VERSION_ID > ...) in the tested code.
8c3d347 to
215d295
Compare
|
This pull request has been marked as ready for review. |
src/Php/PhpVersions.php
Outdated
|
|
||
| private function minPhpVersion(int $versionId): TrinaryLogic | ||
| { | ||
| if ($this->minVersionId >= $versionId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't sufficient. Let's say something is supported only between 7.2 and 7.3. If our intervals only allow < 7.1 and > 8.0, it's not okay to collapse all the intervals, it loses the information.
I know this is just theoretical but let's address this now before this becomes a problem in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized we could just use the all-problem-solving Type->isSuperTypeOf, which means the PhpVersions class effectively no longer contains real logic, which requires a separate unit-test, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still want a test that tests the public method for no/maybe/yes. Thanks.
f35e891 to
f32eec3
Compare
| */ | ||
| public function testRule(int $phpVersion, array $errors): void | ||
| { | ||
| $this->phpVersionId = $phpVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not touch existing test, I can't read this diff. It'd be better to add a completely new file with the top-level ifs. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now tried to keep the old test as much as possible
4387633 to
a2811e7
Compare
9fb6359 to
15f5df9
Compare
15f5df9 to
6b0e638
Compare
|
Awesome, thank you! Now we can go through a lot of occurences of |
|
Also every occurence of This assumes we'll add the used methods to |
| { | ||
| $versionExpr = new ConstFetch(new Name('PHP_VERSION_ID')); | ||
| if (!$this->hasExpressionType($versionExpr)->yes()) { | ||
| return new PhpVersions(new ConstantIntegerType($this->phpVersion->getVersionId())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can afford to start reading min/max values from the phpVersion config.
No description provided.